Instead of adding a new endpoint entirely, can we update EncryptedHostAssignmentContext as follows:

1. rename to HostAssignmentRequest
2. add properties as follows:

public class HostAssignmentRequest
{
    [JsonProperty("encryptedContext")]
    public string EncryptedContext { get; set; }

    [JsonProperty("context")]
    public HostAssignmentContext Context { get; set; }
}

Now callers can choose to supply encrypted or not based on what JSON payload they send. The implementation will then check which of the two properties is not-null and use that.

Hi, Thats a valid suggestion. But, that will break backward compatibility and makes it very difficult to deploy. We would need to change current flow of assignment in Antares outside ATOL. We would have to check which hostversion is supporting which endpoint and send object accordingly.

With new endpoint, we can remove old endpoint once we move to endpoint entirely.

In legion as well, we have seperate endppints for encrypted and unencypted payload to make it easier for maintenance

@manikantanallagatla can you expand more on how my proposal limits maintenance? I am not quite understanding your point.

But, that will break backward compatibility and makes it very difficult to deploy.

How so? One API will serve both formats of JSON. The existing encrypted, or the new unencrypted. The caller can send either of the two formats.

{
  "encryptedContext": "< some encrypted string >"
}

or

{
  "context": {
    "siteName": "MySiteName", // and the rest of the object
   }
}

With new endpoint, we can remove old endpoint once we move to endpoint entirely.

And with my proposal we can remove the support for encryptedContext in the JSON payload. But it is highly unlikely we would remove that, as it would be a breaking change.

What this boils down to, is ATOL can call admin/instance/assign with an unencrypted payload. I don't see any benefit from making it explicitly a different endpoint.

Instead of adding a new endpoint entirely, can we update EncryptedHostAssignmentContext as follows:

1. rename to HostAssignmentRequest
2. add properties as follows:

public class HostAssignmentRequest
{
    [JsonProperty("encryptedContext")]
    public string EncryptedContext { get; set; }

    [JsonProperty("context")]
    public HostAssignmentContext Context { get; set; }
}

Now callers can choose to supply encrypted or not based on what JSON payload they send. The implementation will then check which of the two properties is not-null and use that.

Hi, Thats a valid suggestion. But, that will break backward compatibility and makes it very difficult to deploy. We would need to change current flow of assignment in Antares outside ATOL. We would have to check which hostversion is supporting which endpoint and send object accordingly.

With new endpoint, we can remove old endpoint once we move to endpoint entirely.

In legion as well, we have seperate endppints for encrypted and unencypted payload to make it easier for maintenance

Oh got it. There is a HostAssignmentContext inside HostAssignmentRequest. I missed that. Apologies.

We can make it a single request with this. I will give it a try. Thanks @jviau

@manikantanallagatla I see you re-requested my review, but I don't see the changes we discussed. There are still 2 separate APIs and encrypted/unencrypted models are not combined into the same top-level type. Are these changes still coming?

@jviau just for my curiosity, why do we want to hack into existing api and break the request model object structure from current flow and combining multiple properties where only one is used at any given time, instead of keeping the cleaner segregation with different APIs and their models, and not breaking/modifying the current API on the way.

@manikantanallagatla let's ensure any changes here are aligned by everyone and thought through, because I think we might be making some changes on Legion Platform side or Go Server code to call these APIs while maintaining backward compatibility. Might have to change all those if we have to retrofit new behavior into existing API without breaking the current flows.

@jviau , I merged the APIs into 1 API and requested review. Then when I discussed with Bala, we wanted two separate APIs so that it is cleaner. So, I again reverted that commit of merging into 1 API. Anyways I will start a thread in teams to have a conclusion on 1 API vs 2 APIs.

@jviau just for my curiosity, why do we want to hack into existing api and break the request model object structure from current flow and combining multiple properties where only one is used at any given time, instead of keeping the cleaner segregation with different APIs and their models, and not breaking/modifying the current API on the way.

I don't see this as a hack at all. It doesn't break anything. We are effectively turning the existing JSON payload into a "One-Of" structure. It is either the encrypted payload or the unencrypted payload, and the existing API will determine what assign flow to use based on the shape of the JSON input. We can add validation in the API handler to assert only one can be set at a time. I do not think having an assign and assign2 API is cleaner in any ways. I think my approach is much cleaner. Because again, the API will not break for existing callers.

Existing callers. This is the same as today, and it will be the same after my suggested changes. There is no change for these callers.

POST "admin/instance/assign"

{
    "encryptedContext": "< encrypted context >"
}

NEW callers wanting unencrypted assign:

POST "admin/instance/assign"

{
    "context":  {
        // unencrypted host assignment context
    }
}

Additional scenario which will result in an HTTP 400. Callers will not be allowed to provide both encrypted and unencrypted context.

POST "admin/instance/assign"

{
    "encryptedContext": "< encrypted context >",
    "context":  {
        // unencrypted host assignment context
    }
}